Skip to content

Conversation

@assignUser
Copy link
Member

@assignUser assignUser commented Jan 11, 2023

Rationale for this change

I will keep this as a draft for now please feel free to suggest additional changes!

What changes are included in this PR?

I have switched to smaller headers for now and removed the first head + closes keyword to avoid redundancy.

@assignUser assignUser changed the title GH-33619 : [Documentation] Update PR template GH-33619: [Documentation] Update PR template Jan 11, 2023
@github-actions
Copy link

@github-actions
Copy link

⚠️ GitHub issue #33619 has been automatically assigned in GitHub to PR creator.

@github-actions
Copy link

⚠️ GitHub issue #33619 has no components, please add labels for components.

@kevingurney
Copy link
Member

kevingurney commented Jan 12, 2023

Looks good to me! Thanks for making this change, @assignUser!

Following up on the discussion about stripping comments from the pull request description when merging, I believe adding the following around line 583 of merge_arrow_pr.py should work:

if self.body is not None:
    # Remove comments (i.e. <-- comment -->) from the pull request description.
    body = re.sub(r"<--.*-->", "", self.body)
    # avoid github user name references by inserting a space after @ 
    body = re.sub(r"@(\w+)", "@ \\1", body)
    commit_message_chunks.append(body)

Of course, there is a chance that a contributor might intend to include a literal use of "<-- comment -->" somewhere in their pull request description, but that's seems like a fairly uncommon scenario.

@jorisvandenbossche
Copy link
Member

Following up on the discussion about stripping comments from the pull request description when merging, I believe adding the following around line 583 of merge_arrow_pr.py should work:

Short term that would be good to do to avoid those long noisy commit messages. Longer term, IMO we should discuss using only the title of the PR.

-->

# Are there any user-facing changes?
### Are there any user-facing changes?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another piece of feedback:

  1. The breaking-change label didn't exist. I decided to add it as Breaking Change just now, since there isn't a good reason to kebab-case AFAIK.
  2. I don't think non-committers can apply labels, right? So perhaps the instructions simply need to have a checkbox [ ] This PR contains breaking changes or something like that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'll handle this in #33660

@assignUser assignUser marked this pull request as ready for review January 16, 2023 00:25
@assignUser assignUser requested a review from raulcd January 16, 2023 00:25
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already a big improvement IMO, so let's merge this

@jorisvandenbossche jorisvandenbossche merged commit 7e6bcd1 into apache:master Jan 16, 2023
@jorisvandenbossche
Copy link
Member

Whoops, sorry, the linter was failing on the code line in archery .. This will be fixed by #15067

@ursabot
Copy link

ursabot commented Jan 16, 2023

Benchmark runs are scheduled for baseline = 7d3bca3 and contender = 7e6bcd1. 7e6bcd1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.39% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.22% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 7e6bcd1b ec2-t3-xlarge-us-east-2
[Finished] 7e6bcd1b test-mac-arm
[Finished] 7e6bcd1b ursa-i9-9960x
[Finished] 7e6bcd1b ursa-thinkcentre-m75q
[Finished] 7d3bca39 ec2-t3-xlarge-us-east-2
[Finished] 7d3bca39 test-mac-arm
[Finished] 7d3bca39 ursa-i9-9960x
[Finished] 7d3bca39 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Documentation] Update PR template

5 participants